-
-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add commands to manage tags via OCC #26600
Conversation
1b8c894
to
260cd19
Compare
lib/public/SystemTag/ISystemTag.php
Outdated
* | ||
* @since 22.0.0 | ||
*/ | ||
public function getHumanReadableAccess(): string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure if we'd want this in the public API. As then it can never be translated.
Also maybe a constant is better than just a string? @ChristophWurst
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wanted to avoid code reuse in the commands. Not sure what would be a better solution? The result of this function could still be used as translation keywords. I wouldn't mind using a constant, but then I'd have to reuse more code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've decided to go ahead and implement a constant-based solution, which feels much cleaner. Please check it out and tell me what you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of minor comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 👍 Cool feature 🥳
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
My other comments are only suggestions. Not required to get it in.
|
list, add, delete, edit Signed-off-by: Johannes Leuker <j.leuker@hosting.de>
I've rebased the branch on master: #27101 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good 👍
I'm adding
tag:list
,tag:add
,tag:delete
, andtag:edit
commands to OCC